Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Don't autosave on focus-in to editor #4387

Closed
wants to merge 1 commit into from

Conversation

novaugust
Copy link
Contributor

Closes #4321

  • Added a PostTitleInputComponent that sets its value to "(Untitled)" if it has no value on focusOut, and selects its text if its text is "(Untitled)" on a click.
  • Removed autoSaveNew action firing onFocusIn of the markdown editor
  • emberified the gh-trim-focus-input
  • editor-base-controller is no longer responsible for setting a default title (horay!)

@novaugust novaugust force-pushed the autosave-tweakin/4321 branch 2 times, most recently from b2eeb6b to db5f2dd Compare November 5, 2014 04:09
@novaugust
Copy link
Contributor Author

Updated to pass tests

  • if a post has no body and its title is (Untitled), a user will not be asked to save before leaving - it just gets thrown away
  • Removed "ohnoes autosave" code from writeContentToCodemirror functional test helper introduced in fbccc36 .
    Someone double check me on the removal of that. I think it's fine, as that code was written to handle the "focus-in -> autosave", which no longer happens. But, do we need to worry about 3-second-pause autosave down the road?

@novaugust
Copy link
Contributor Author

"updated to fix tests", he says. note to self: if you fix one, check you don't have broken ones that never got the chance to run

@novaugust novaugust force-pushed the autosave-tweakin/4321 branch from db5f2dd to 1119e31 Compare November 5, 2014 04:21
@novaugust
Copy link
Contributor Author

Okay, I was feeling very positive about this post, but now there's one thing that's feeling a bit janky:

If you go to New Post, then click Content without doing anything else, you'll see (Untitled) flash into the input then go poof as we transition.

untitled poof

A proposed solution: I'm going to set titleScratch to (Untitled) from the start, and instead of just focusing, have the text selected, so any bit of typing will kill it. Update en route, unless it breaks tests, in which case this'll come out tomorrow :)

@ErisDS
Copy link
Member

ErisDS commented Nov 5, 2014

This was the reason why we moved the autosave onto focusIn of the editor, rather than focusOut of the title - as the title is auto-focused, so it will always happen if you hit the editor and go elsewhere.

Closes TryGhost#4321
- Added a PostTitleInputComponent that sets its value to "(Untitled)" if it has no value on focusOut, and selects its text if its text is "(Untitled)" on a click.
- Posts have a default title, '(Untitled)'
- Removed autoSaveNew action firing onFocusIn of the markdown editor
- emberified the gh-trim-focus-input
- editor-base-controller is no longer responsible for setting a default title (horay!)
- Remove pause for autosave from functional-test helper "writeContentToCodeMirror"
@novaugust novaugust force-pushed the autosave-tweakin/4321 branch from 1119e31 to 73b8ce8 Compare November 7, 2014 15:29
@novaugust
Copy link
Contributor Author

Before i spend a few hours in test-fixup land, I'd love for someone to take this for a spin and objectively state whether it's worth it or not. Both the UX (when you're asked to save a post, and how the default title pops in) and code (fiddly, arbitrary looking bits) need review to say if this is something we actually want to introduce, or if it's just good as-is ;)

selectOnInsert: function () {
this.$().select();
}.on('didInsertElement')
});

This comment was marked as abuse.

This comment was marked as abuse.

@ErisDS
Copy link
Member

ErisDS commented Nov 11, 2014

So there's been much toing and froing around this centring on what and when to set the title if it isn't set by the user (which has always been the difficulty with autosave).

What we have now is the first autosave will set the title to '(Untitled)' if one is not already set. That can be quite jarring if it happens at a random point (as autosave does), so we try to set the title during the user's focus change from the title to the textarea.

  • Basing this on focusOut from the title means it gets set even if you navigate to the editor and away again - which is weird and ugly.
  • Basing this on focusIn of the textarea works OKish, but makes for less pretty ugly code.

All this has ended up boiling down to the question 'Why are we setting the title anyway?'

I think that allowing blank titles in drafts, and then throwing an error if a user tries to publish (or in future, schedule) doesn't make for a good UX.

If this is deemed not to be the case, the suggestion is that we could leave the title blank in the editor, use '(Untitled)' as the title in the content list and this would simplify the editor code.

However, even if we aren't setting the title, the first autosave behaves a little bit differently from all other autosaves, causing the URL to update and the page to rerender (and in some unfortunate browsers, the modal to fire #4400). I think this experience is still going to be jarring, even without the title update because it will actually lose a few characters of what the user is typing.

Essentially, I think the first autosave is tricky and that we still have to solve the problem of when to do it to provide a less jarring experience. I'm pretty sure that when the user is changing their focus into the textarea is the best time to do this, unless we do it on load, and delete the post if the user doesn't change anything?

@novaugust novaugust changed the title Don't autosave on focus-in to editor [WIP] Don't autosave on focus-in to editor Nov 17, 2014
@novaugust
Copy link
Contributor Author

I think it's okay to let this PR go now. It's illuminated some odd bits in our editor transitions and shared code between disparate controllers. I'm really starting to think that a good solution to any autosave issues will become much more apparent and easier to implement with a better foundation to build off of, where we don't have to worry about so many quirks.
I'd ultimately like to have a big ol' refactor-fest that combines the editor/new and editor/edit routes/controllers into a single route. That would make saving a new post a seamless operation, and, I think, make it easier to refactor (perhaps even combine) the post-settings-menu into the editor as well. (See our implementation of settings/tags for a good example of that)

@novaugust novaugust closed this Dec 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autosave is overly ambitious
2 participants